-
Notifications
You must be signed in to change notification settings - Fork 5
fix: update OpenAttestationEmbeddedRenderer to EmbeddedRenderer #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
index.html
Outdated
|
|
||
| <section> | ||
| <h4>OpenAttestationEmbeddedRenderer</h4> | ||
| <h4>DecentralizedRenderer</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A naming question here. What about the renderer itself makes it decentralized?
From the description, it sounds like this is a renderer that uses HTML in an embedded or nested browsing context, in particular, it suggests an iframe must be used. If the latter is true (and intended to be true carrying forward), perhaps IFrameRenderer would be an appropriate name to describe why/how this renderer is different from the other renderer types.
If there's some other layer to the abstraction here and use of an iframe is an implementation detail, perhaps it suggests the name ought to be ExternalRenderer or EmbeddedRenderer -- where there is some External/Embedded "ACTION" API to communicate with it?
If the main feature of this type of renderer is that it has this "ACTION" API communication capability, perhaps the name should be based on that, e.g., ActionableRenderer or ActionDrivenRenderer (or similar). Perhaps those names are slightly awkard or insufficiently specific -- but I wouldn't suggest InteractiveRenderer since "interactivity" could be a property of many different renderer types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thoughtful feedback!
After some discussion within the team, we agree that the name should stay close to how the renderer actually works. Based on your suggestions, we think EmbeddedRenderer would be the most accurate and intuitive choice, since it reflects that the rendering happens in an embedded context.
We’ll proceed with updating the naming accordingly.
|
Hi @dlongley, Thanks for the feedback, have updated it to be EmbeddedRenderer instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to figure out the context issue in this PR.
- The PR should be updated to refer to the new issue on the topic.
- The images need descriptions for screen readers (for people that have low/no-vision sight needs). I have used LLMs to generate these descriptions and they do a pretty good job (sometimes) if you give them enough context on what the image is showing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
Issue #33 (to deal with the |
Rename "OpenAttestationEmbeddedRenderer" to "EmbeddedRenderer".
As there is an update to OA.
Preview | Diff